Repository navigation
New signals - #92
New signals#92trueroguan wants to merge 4 commits into
Conversation
PR Summary by QodoAdd threshold-gated camera alerts and external actions
AI Description
Diagram
High-Level Assessment
Files changed (43)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
Resolve comments and we will run workflow to doublecheck |
keyldev
left a comment
keyldev
left a comment
There was a problem hiding this comment.
Thanks for this, it's a solid piece of work. The threshold and episode logic in NotificationCoordinator holds up: thresholds are checked on every publication, each channel fires at most once per episode, and Core still has no package dependencies. The P/Invoke and COM layouts (WinRT toasts, CREDENTIALW, MAPI) check out, and Windows-only code is properly guarded. Locally the PR builds with 0 warnings.
CI is red for an unrelated reason: a new advisory on System.Security.Cryptography.Pkcs 10.0.0 (GHSA-2fmv-rv2c-59x7, published 2026-10-13) fails restore on main as well. That will be fixed on main; rebase once it lands.
A few things need changing before merge.
Must fix
-
Command injection in the "run program" action (
EventActionDispatcher.cs,StartProgram).ProgramArgumentsis rendered and then passed as one raw command-line string..bat/.cmdgo throughcmd /c, so& | > ^ %in the values are interpreted by the shell.- The default template quotes
"{camera}", but the camera name isn't always typed by the user. Auto-detect fills it from the name the device reports (CameraEditorViewModel). A device on the LAN that calls itselfx" & calc & "runscalcon every event. - The
.ps1and ShellExecute branches have the same quoting break. - Fix: split the template into tokens before rendering, render each token separately, and pass them through
psi.ArgumentList. For.bat/.cmd, also reject or strip"%&|<>^and line breaks in substituted values, or pass event data only through theOPENIPC_*environment variables. Those variables are currently not set on the ShellExecute branch, although the comment says scripts can rely on them.
-
Event data can inject bytes into TCP/UDP payloads (
SendSignalAsync).- The template is rendered first and the escapes are decoded after, so a camera named
Gate\r\nDISARMor containing\x00puts raw bytes into an alarm panel or PLC stream. - In
hex:mode, a placeholder value that isn't hex throws. - Fix: decode escapes in the template before substituting values (or escape
\in the values), and reject placeholders in hex mode.
- The template is rendered first and the escapes are decoded after, so a camera named
-
SMTP "auto" can send the password in plain text (
SmtpMailSender.cs).- On any port other than 465, "auto" maps to
StartTlsWhenAvailable. Someone in the network path can strip the STARTTLS offer, and AUTH then goes out unencrypted. - Fix: when a login is configured, "auto" should use
StartTls, which fails if STARTTLS is missing.
- On any port other than 465, "auto" maps to
Should fix
- The Events page does a full refilter every second per active camera.
EventIngestionServicenow publishes progress about once a second and on every peak rise.EventsPageViewModel.OnLiveEventrunsApplyFilterover up to 10,000 events on the UI thread for each update. With several AI cameras active, that is several full rebuilds per second. Mark progress publications so the Events page can skip them, or throttleOnLiveEvent. - An open event can arrive after the close.
OpenAsyncpublishes only afterawait _repo.AddAsync, while the quiet-close timer runs independently. If the insert takes more than 5 s, the coordinator sees the close first and removes the episode. The late open event then creates a new episode that is never removed, and it notifies or acts a second time. Mark the state as closed and skip the late publish, or keep a short set of recently closed IDs in the coordinator. - An unsaved SMTP password is silently lost.
LoadAsyncruns each time Settings is shown and overwritesSmtpUser/SmtpPasswordfrom the store.FlushAsyncdoesn't save dirty credentials. A user who types a password and navigates away without pressing Save/Test gets empty fields back, and every e-mail action then fails authentication. Save the credentials inFlushAsyncwhen they're dirty, or don't overwrite dirty fields on load. - "Test notification" can crash the app.
TestNotificationAsyncawaitsSaveNowAsync()without a try/catch. An IOException (antivirus or OneDrive lockingusersettings.json.tmp, a full disk) escapes theAsyncRelayCommandand takes the process down. The other Test commands catch their errors. - Importing a big WAV freezes the UI.
WavCodec.Readruns synchronously on the UI thread and reads the entiredatachunk before trimming to 15 s. A 1-hour WAV allocates hundreds of MB, and a file over 2 GB overflows the(int)cast into anArgumentOutOfRangeException, which doesn't fall back to FFmpeg. Cap the read atMaxCustomLengthand run it inTask.Run. Also reject absurd sample rates: a rate of 2^30 overflowsBytesPerSecondto 0 and leaves the player loop spinning. - MAPI and Outlook sends are unbounded. Each send gets a new STA thread that can't be cancelled; the 20 s timeout only stops waiting for it. A compose window or an Outlook security prompt keeps that thread alive, and a burst of events stacks up threads and dialogs. Allow one send at a time.
- Phone widths. The Test-button rows put a status
TextBlock MaxWidth="420"inside a horizontalStackPanel, so long errors are clipped at about 360 dp. Fixed-width sliders also crowd the labels. Use aGrid(Auto,*) or put the status on its own line, and useb:Adaptive.CompactBelowlike the other sections.
Nits
- Cooldowns compare local wall-clock time. On the DST fall-back night
now - atgoes negative and blocks both channels for about an hour. Use UTC for cooldowns and local time only for quiet hours. - Actions are gated by the toast toggles (
NotifyOnMotion/NotifyOnDetection) but ignore theNotificationsEnabledmaster switch. Turning off motion toasts silently turns off motion actions too. Either give actions their own filter or document this. WindowsToastNotifier.ShowafterDisposethrowsInvalidOperationException.RunAndWaitcatches this;Showdoesn't.- The "Not an http(s) URL" warning logs the full webhook URL, which often carries a token. Log only the scheme and host.
- The
.ps1branch always runspowershell.exe. Usepwshoff Windows, or take this branch only on Windows. NotificationSoundPlayernever disposes supersededCancellationTokenSources, andScheduleSavehas the same leak.- The program-path hint and the README describe MAPI, Outlook and Credential Manager without saying they're Windows-only. Off Windows, "system secure storage" is actually the app's
secrets.bin. SettingsPageViewModel.Persiststill uses the snapshotUpdateAsync(...), which can overwrite a notification save that is still in flight. Switch it to the functional overload.
|
The current Qodo review is complete: 0 active findings. All 7 previously reported findings are marked resolved. |
|
Thanks for the update. I went through 07c4897..4837e2b against the previous review, and every point in it is addressed:
There's one behaviour change I'm fine with but want to state explicitly. A channel that is held back by its cooldown or quiet hours can now fire later in the same episode, once the gate opens. What's left: Should fix
|
Summary
Extends camera-event notifications and adds external actions:
Related
Phase 19 — Community & app-level (19.3 Notifications).
Type
Checklist
TreatWarningsAsErrors=true).dotnet test); new Core logic has unit tests.AppreferencesCoreonly (Infrastructure / Video / Devices wired via DI in a head).Platforms tested
Screenshots / notes
SshSessionIntegrationTestsconstructor call.🤖 Generated with Claude Code